-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
stats: Improved sequencing documentation for server-side stats events and added tests. #7885
base: master
Are you sure you want to change the base?
Conversation
…s to verify the order of server-side events.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7885 +/- ##
==========================================
+ Coverage 81.84% 82.09% +0.24%
==========================================
Files 377 381 +4
Lines 38120 38539 +419
==========================================
+ Hits 31201 31639 +438
+ Misses 5603 5582 -21
- Partials 1316 1318 +2
|
stats/stats_test.go
Outdated
func (s *testServer) UnaryCall( | ||
ctx context.Context, | ||
in *testpb.SimpleRequest, | ||
) (*testpb.SimpleResponse, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
grpc-go follows the Google style guide. Based on this section in the style guide:
The signature of a function or method declaration should remain on a single line to avoid indentation confusion.
Please revert the changes made to wrap function definationations and calls throughout the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! I'll disable my autoformatter going forward or reconfigure it to follow the Google style guide.
stats/stats_test.go
Outdated
@@ -786,8 +884,14 @@ func checkConnEnd(t *testing.T, d *gotData) { | |||
st.IsClient() // TODO remove this. | |||
} | |||
|
|||
type event struct { | |||
eventType string | |||
timestamp time.Time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the use of storing a timestamp? I believe we're only interested in the ordering of the events and not the absolute times they were emitted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The timestamp was included for debugging purposes in case of test failures, since it could help identify delays or other issues. If you feel this isn’t valuable, I can to remove it for simplicity.
I have fixed the wrapping errors for formatting and I removed the timestamps from the event structs. Thank you for your feedback @arjan-bal, is there anything else you need me to do? |
…. Added a server stats Server Stream RPC test.
stats/stats_test.go
Outdated
@@ -81,6 +83,55 @@ var ( | |||
} | |||
// The id for which the service handler should return error. | |||
errorID int32 = 32202 | |||
|
|||
// Server Stats |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the documentation everywhere. Remove extra spaces at start and end etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything should be resolved now. Thank you for your help!
I fixed the autoformatting mistakes and cleaned up the comments. I will not use an autoformatter going forward, since it caused me a headache. Is there anything else needed in terms of testing? |
Thanks, I will take a look. Looks like you have some vet check failures. Could you run the vet check locally and fix https://github.com/grpc/grpc-go/blob/master/CONTRIBUTING.md ? |
The vet errors should be fixed now. Happy holidays! |
I'm still failing one test for some reason. I didn't see that locally. I'll look into it when I get the chance. |
stats/stats.go
Outdated
@@ -36,7 +36,10 @@ type RPCStats interface { | |||
IsClient() bool | |||
} | |||
|
|||
// Begin contains stats when an RPC attempt begins. | |||
// Begin contains stats when an RPC attempt begins. This event is called after | |||
// the InHeader event, as headers must be processed before the RPC lifecycle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit InHeader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What specifically is the issue here? Is it related to style?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Names from code should preferably be within backticks
stats/stats.go
Outdated
@@ -98,7 +101,9 @@ func (s *InPayload) IsClient() bool { return s.Client } | |||
|
|||
func (s *InPayload) isRPCStats() {} | |||
|
|||
// InHeader contains stats when a header is received. | |||
// InHeader contain stats when the header is received. It is the first event in | |||
// the server after receiving the RPC. It is followed by the OutPayload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OutPayload
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify what you are asking for here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Names from code should preferably be within backticks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, isn't it followed by Begin?
stats/stats_test.go
Outdated
@@ -81,6 +81,50 @@ var ( | |||
} | |||
// The id for which the service handler should return error. | |||
errorID int32 = 32202 | |||
// To verify if the Unary RPC server stats events are logged in the | |||
// correct order. | |||
expectedUnarySequence = []string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be initialized within the test itself because we are not using it anywhere else. Also, convention is to use want/wanted
instead of expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! I will keep this in mind going forward. Thank you!
…nally used cmp.Equal rather than iterating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking close to LGTM. Few last set of comments.
stats/stats.go
Outdated
@@ -98,7 +101,9 @@ func (s *InPayload) IsClient() bool { return s.Client } | |||
|
|||
func (s *InPayload) isRPCStats() {} | |||
|
|||
// InHeader contains stats when a header is received. | |||
// InHeader contain stats when the header is received. It is the first event in | |||
// the server after receiving the RPC. It is followed by the OutPayload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, isn't it followed by Begin?
switch s.(type) { | ||
case *stats.Begin: | ||
h.recordEvent("Begin") | ||
case *stats.InHeader: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may be InHeader should be first case to be consistent with the order of occurence
stats/stats_test.go
Outdated
gotEventTypes[i] = e.eventType | ||
} | ||
if !cmp.Equal(gotEventTypes, expected) { | ||
t.Errorf("Event sequence mismatch (-got +expected):\n%s", cmp.Diff(gotEventTypes, expected)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/expected/want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diff (-got +expected)
since you are showing the diff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming by added the vim command you want me to replace all occurrences of 'expected.'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is followed by Begin for server-side stats events. It follows the OutPayload for client-side events. I need to clarify this.
stats/stats_test.go
Outdated
|
||
// verifyEventSequence verifies that a sequence of recorded events matches | ||
// the expected sequence. | ||
func verifyEventSequence(t *testing.T, got []event, expected []string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/expected/want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are lines that I didn't write that contain the word 'expected.' Should those be changed too?
stats/stats_test.go
Outdated
type statshandler struct { | ||
mu sync.Mutex | ||
events []event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like we just need list of event types? Can't we directly use []string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that should suffice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unblocking for other reviewers to approve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Undo approval.
Approved while trying to remove the change request.
Response to Issue #7824
RELEASE NOTES: